Skip to content

Conversation

@sbordet
Copy link
Contributor

@sbordet sbordet commented Oct 17, 2025

No description provided.

@sbordet sbordet moved this to 👀 In review in Jetty 12.1.4 Oct 17, 2025
@sbordet sbordet requested review from joakime and lorban October 17, 2025 14:48
OBF:1yta1t331v8w1v9q1t331ytc <3>
MD5:5eBe2294EcD0E0F08eAb7690D2A6Ee69 <4>
...
MD:SHA-1:E5E9Fa1bA31eCd1aE84f75CaAa474f3a663f05F4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OBF can be reversed to a password.
MD5, SHA1-, SHA-256 is 1 direction, it cannot be reversed into a password.

Is that nuance worth mentioning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This nuance is important for reversible passwords.

A stored password for a database connection? needs to be reversible, so that the code can actually submit that password to the database server.

A stored password for verifying a submitted password (like a user login), that can be a MD based 1-way password.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's almost like describing a stored password as verifying an incoming password (all storage types supported) or obtaining an outgoing password (only reversible supported)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joakime we always had MD5, so this PR adds just a pluggable format for MD-based checksums.

I have added in the docs an example for 1-way password.

Where do you think we need to add more in the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this isn't a need for a code change, it feels more documentation specific.
I'll rereview the docs shortly.

joakime
joakime previously approved these changes Oct 22, 2025
lorban
lorban previously approved these changes Oct 23, 2025
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change LGTM, but I would like to remind that keeping passwords as String objects is not very secure as they would be included in a heap dump for instance, so they should be kept in char[] and the array should be wiped immediately after use.

That's probably a non-issue for a short-lived command-line tool like the one here, though. And this would need to be solved in a different PR.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet dismissed stale reviews from lorban and joakime via f82e7da October 24, 2025 16:33
Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet merged commit 7d3573a into jetty-12.1.x Oct 24, 2025
2 of 4 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.1.4 Oct 24, 2025
@sbordet sbordet deleted the fix/jetty-12.1.x/improve-credential branch October 24, 2025 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants